-
Notifications
You must be signed in to change notification settings - Fork 5
wip: fix: resize with interpolationType nearest #453
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@stropitek wdyt? Maybe we should remove the custom code and use |
I think it would make sense to use |
I would like to try to use transform matrix instead but I don't know how it work |
https://en.wikipedia.org/wiki/Scaling_(geometry) With a 2x3 matrix, it should be
|
It's the best I can do. I let you review before resetting snapshots to let the tests pass. |
Do we still need custom implementation of the interpolation functions? |
Bilinear is known to give different result compared to OpenCV. Solution is to add a margin of error with comment like here: image-js-typescript/src/geometry/__tests__/transform.test.ts Lines 110 to 113 in 8d81c2b
Use the smallest possible error margin |
I removed It because I do not use it anymore.
Ok I thought we should be one to one with OpenCV (And I found his less precise algorithm more smoother). If you agree with theses code changes I think we can reset snapshots and merge |
Really? The difference is barely visible in the other tests (you have to quickly change from one image to the other to see that the colors change, but they are basically the same). |
Refs: #453 (comment) Refs: #453 (comment) Refs: #453 (comment)
I did not check other tests, but take a lot of time to tweak trying to get result most similar result with opencv. |
After snaptshots reset, some tests did not pass yet |
There are still substantial changes to interpolateNeighbourBilinear which may explain it. |
Yes, I'll adapt tests data |
But are you sure about the change to the algorithm? It worked fine until now. |
I'm not sure. Maybe I should rollback the bilinear but keep nearest. |
const { | ||
interpolationType = 'bilinear', | ||
borderType = 'constant', | ||
borderType = interpolationType === 'bilinear' ? 'replicate' : 'constant', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Is it OpenCV behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know but bilinear was better with replicate by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably not the right way to fix it.
// OpenCV bilinear interpolation is less precise for speed. | ||
expect(resized).toMatchImage('opencv/testResizeBilinear.png', { error: 25 }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error: 25
is too much (that's 10% of the range of values 0-255)
This PR was in a dead end I preferred to restart from scratch and only fix nearest resize. This PR will be usefull for future as exploration of this topic. |
Closes: #452